Skip to content

fix: add missing functional test #1883

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
yuki-97 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: add missing functional test #1883

yuki-97 wants to merge 3 commits into from
+53 −61

Conversation

@yuki-97
Copy link
Contributor

@yuki-97 yuki-97 commented Feb 5, 2026 •
edited
Loading

  1. remove tests/functional/grpo_math_env.sh since duplicated with tests/functional/grpo.sh.
  2. add missing functional tests:
    1. tests/functional/dpo_megatron.sh
    2. tests/functional/grpo_rm_env.sh
      1. this one is disabled since it's currently broken in CI.
      2. fix: fix and re-enable rm env functional test #1905 will fix it.
    3. tests/functional/sft_megatron.sh
    4. tests/functional/test_converters.sh
      1. this one is disabled since DTensor v2 converter is not supported now.
      2. fix: Fix DCP-to-HF conversion for model-wrapped checkpoints #1881 will support it.
  3. add unit test to prevent.

Summary by CodeRabbit

  • Tests
    • Reorganized functional GPU test suite with expanded coverage including new distillation, DPO, and evaluation tests.
    • Removed one redundant test script to streamline test execution.
    • Updated test configurations for improved stability and accuracy.
    • Added validation to ensure all functional tests are properly tracked.

@yuki-97 yuki-97 requested a review from a team as a code owner February 5, 2026 16:10
@yuki-97 yuki-97 added the CI:L1 Run doctests, unit tests, and functional tests label Feb 5, 2026
@yuki-97 yuki-97 requested a review from terrykong February 5, 2026 16:11
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 5, 2026 •
edited
Loading

📝 Walkthrough

Walkthrough

The PR reorders and expands the GPU functional test suite by replacing and adding test invocations, updates a Megatron DPO configuration file with a v2 config path and additional flags, removes the grpo_math_env.sh test script, and adds a unit test to ensure all functional test scripts are referenced in the main test orchestration script.

Changes

Cohort / File(s) Summary
Functional GPU Test Suite
tests/functional/L1_Functional_Tests_GPU.sh, tests/functional/dpo_megatron.sh		 Reordered and expanded functional test invocations; replaced SFT-based tests with distillation, DPO, and evaluation tests earlier in suite. Updated Megatron DPO config from v1 to v2 with sequence parallel flag; increased loss threshold from 5 to 6.
Removed Tests
tests/functional/grpo_math_env.sh		 Deleted entire functional test script for GRPO math environment setup and execution.
Test Coverage Validation
tests/unit/test_recipes_and_test_suites.py		 Added new unit test function to verify all functional test scripts in tests/functional/ are referenced in the main L1_Functional_Tests_GPU.sh orchestrator.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

CI:L1

Suggested reviewers

  • terrykong
  • yaoyu-33
  • chtruong814
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: add missing functional test' is partially related to the changeset. It refers to adding missing functional tests, which is a real aspect of the change, but it's somewhat incomplete—the PR also involves removing a duplicate test (grpo_math_env.sh) and reordering/reorganizing the test suite.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Test Results For Major Changes ✅ Passed PR contains minor test suite maintenance changes including removing duplicates, adding missing tests, and reorganizing test execution without major product changes.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch yukih/functional-test

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

terrykong
terrykong previously approved these changes Feb 5, 2026
View reviewed changes
@terrykong terrykong enabled auto-merge (squash) February 5, 2026 17:34
@yuki-97 yuki-97 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels Feb 6, 2026
@yuki-97 yuki-97 requested a review from terrykong February 6, 2026 04:36
terrykong
terrykong previously approved these changes Feb 6, 2026
View reviewed changes
@yuki-97 yuki-97 force-pushed the yukih/functional-test branch from 1575a0a to 9550f8c Compare February 9, 2026 02:59
@yuki-97 yuki-97 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels Feb 9, 2026
@yuki-97 yuki-97 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels Feb 9, 2026
@yuki-97
add missing test
fa9da0f 
Signed-off-by: Yuki Huang <yukih@nvidia.com>
@yuki-97
fix node
fba72bb 
Signed-off-by: Yuki Huang <yukih@nvidia.com>
@yuki-97 yuki-97 force-pushed the yukih/functional-test branch from a8e7037 to fba72bb Compare February 10, 2026 02:59
@yuki-97 yuki-97 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels Feb 10, 2026
terrykong
terrykong previously approved these changes Feb 10, 2026
View reviewed changes
@yuki-97
disable grpo_rm_env
a2ea2b2 
Signed-off-by: Yuki Huang <yukih@nvidia.com>
@yuki-97 yuki-97 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels Feb 10, 2026
@yuki-97 yuki-97 requested a review from terrykong February 10, 2026 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@terrykong terrykong Awaiting requested review from terrykong

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

CI:L1 Run doctests, unit tests, and functional tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yuki-97 @terrykong